New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check basic auth credentials before authenticate #43209
Conversation
82dabac
to
18fe6fd
Compare
test "authentication request with credentials without colon" do | ||
@request.env["HTTP_AUTHORIZATION"] = "Basic #{::Base64.encode64("David Goliath")}" | ||
get :search | ||
assert_response :unauthorized | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the credentials "David Goliath" are invalid as well, maybe it's better to move the test to a specific has_basic_credentials?
test like we do for encode_credentials
:
rails/actionpack/test/controller/http_basic_authentication_test.rb
Lines 107 to 113 in 5462fbd
def test_encode_credentials_has_no_newline | |
username = "laskjdfhalksdjfhalkjdsfhalksdjfhklsdjhalksdjfhalksdjfhlakdsjfh" | |
password = "kjfhueyt9485osdfasdkljfh4lkjhakldjfhalkdsjf" | |
result = ActionController::HttpAuthentication::Basic.encode_credentials( | |
username, password) | |
assert_no_match(/\n/, result) | |
end |
Something like:
test "authentication request with credentials without colon" do | |
@request.env["HTTP_AUTHORIZATION"] = "Basic #{::Base64.encode64("David Goliath")}" | |
get :search | |
assert_response :unauthorized | |
end | |
test "authentication request with credentials without colon" do | |
@request.env["HTTP_AUTHORIZATION"] = "Basic #{::Base64.encode64("David Goliath")}" | |
refute ActionController::HttpAuthentication::Basic.has_basic_credentials(@request) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I changed the test to specific for has_basic_credentials?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 😄 I have one last change request.
Could you move it under the test_encode_credentials_has_no_newline
test?
This will group the tests that don't do get
requests together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course! 🙂 I moved it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 😄
18fe6fd
to
8dadd2b
Compare
8dadd2b
to
22e6cb2
Compare
Check basic auth credentials before authenticate
I don't think this patch is quite correct. it's perfectly legitimate to connect with a username but no password. |
rails#43209 immediately rejects the request if no password is passed, but there are legitimate uses for accepting authentication without a password.
#43209 immediately rejects the request if no password is passed, but there are legitimate uses for accepting authentication without a password.
…ntials" This reverts commit e4ad4a2.
@casperisfine Yeah! You are right, I did not support using the access token as username and the password it is not mandatory in this case. |
This would through an error in development or test environments with something like: http_basic_authenticate_with name: Rails.application.credentials.dig(:testing, :username), \
password: Rails.application.credentials.dig(:testing, :password), if: -> { Rails.env.production? } Since the Can be resolved by defining empty username and password credentials for non-production environments, but still rather unexpected to have a callback throw an error before or without the conditional being satisfied. |
Summary
If you send a request to a controller protected by basic authentication with wrong credentials (without the colon) you will get the error:
NoMethodError: undefined method 'bytesize' for nil:NilClass
.For example:
Stacktrace: